Upgrade helix-core to 1.4.3 to address CVE-2023-38647 by ShahimSharafudeen · Pull Request #25812 · prestodb/presto

ShahimSharafudeen · 2025-08-18T16:37:27Z

Description

Upgraded helix-core to version 1.4.3 to address CVE-2023-38647, as the vulnerable version (helix-core:1.0.4) was introduced as a transitive dependency through the presto-pinot-driver.

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

imjalpreet

@ShahimSharafudeen, thank you. Why don't we add this to the root pom's DependencyManagement?

steveburnett · 2025-08-19T13:48:13Z

Thanks for the release note! Minor formatting nit.

== RELEASE NOTES ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

ShahimSharafudeen · 2025-08-19T15:50:31Z

@ShahimSharafudeen, thank you. Why don't we add this to the root pom's DependencyManagement?

@imjalpreet - This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

ShahimSharafudeen · 2025-08-19T15:51:49Z

Thanks for the release note! Minor formatting nit.

== RELEASE NOTES ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

Thanks @steveburnett for the correction. I've made the changes accordingly.

imjalpreet · 2025-08-20T22:43:13Z

This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

@ShahimSharafudeen IMO, I’d still suggest adding it to the root POM. This way, any future additions to other modules will be automatically covered, and it also simplifies version management across multiple modules.

ShahimSharafudeen · 2025-08-21T07:06:37Z

This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

@ShahimSharafudeen IMO, I’d still suggest adding it to the root POM. This way, any future additions to other modules will be automatically covered, and it also simplifies version management across multiple modules.

@imjalpreet - Agreed with your point. I’ve moved the dependency to the root POM to ensure future modules are automatically covered and to simplify version management across the project.

imjalpreet

Thanks, LGTM, a small nit.

imjalpreet · 2025-08-21T10:44:41Z

+            <dependency>
+                <groupId>org.apache.helix</groupId>
+                <artifactId>helix-core</artifactId>
+                <version>1.4.3</version>


nit: please add the version as a property

imjalpreet · 2025-08-21T10:45:34Z

+            <dependency>
+                <groupId>org.apache.helix</groupId>
+                <artifactId>helix-core</artifactId>
+                <version>1.4.3</version>
+            </dependency>


Also, add a comment to explain why we have added this here.

imjalpreet

@ShahimSharafudeen, thank you, it LGTM, just a small suggestion to move this dependency just after the pinot-driver dependency here:

presto/pom.xml

Lines 2242 to 2246 in e53f403

    
           <dependency> 
        
               <groupId>org.apache.pinot</groupId> 
        
               <artifactId>presto-pinot-driver</artifactId> 
        
               <version>${dep.pinot.version}</version> 
        
           </dependency>

imjalpreet · 2025-08-25T12:22:03Z

                </exclusions>
            </dependency>

+            <!-- Add the helix-core dependency to the root pom.xml to upgrade the transitive helix-core version used by the Presto Pinot driver-->


Suggested change

@imjalpreet – As per your request, I’ve updated the comment details based on your suggestion.

imjalpreet

Thanks, @ShahimSharafudeen

tdcmeehan

Is there an update to the Pinot driver which fixes this CVE?

imjalpreet · 2025-08-29T16:18:18Z

Is there an update to the Pinot driver which fixes this CVE?

@tdcmeehan, I had checked this earlier. There hasn't been any new version released since March 2023, and the latest version is still using helix-core:1.0.4.

https://mvnrepository.com/artifact/org.apache.pinot/presto-pinot-driver/0.12.1

tdcmeehan · 2025-08-29T16:19:49Z

Can we create an issue in Pinot and link to that issue here? And potentially contribute this change upstream to the Pinot driver?

imjalpreet · 2025-08-31T09:09:42Z

@tdcmeehan, the presto-pinot-driver was deprecated by Pinot in 2023 when they moved to a higher JDK version, but at that time we were still on JDK 8. Instead of continuing with the presto-pinot-driver, we needed to upgrade to the latest Pinot libraries, but the Java version held us back until now.

This PR was originally intended as a temporary CVE fix for the existing implementation we had added at IBM before upgrading to JDK 17.

Now that we’re on Java 17, the Pinot library upgrade is already underway. We’re moving to the latest libraries and removing the deprecated presto-pinot-driver. I checked the status, and it’s further along than I initially thought when I reviewed a couple of weeks back, so it might make sense to hold off on merging this PR and instead complete the upgrade, especially since that process already includes bringing the helix-core:1.4.3 dependency into Presto.

Pinot library upgrade PR: #25785

ShahimSharafudeen · 2025-09-09T14:13:18Z

Thanks, @imjalpreet , for your clarification on the Pinot driver change during my absence.

@tdcmeehan @imjalpreet — Could you please confirm whether this PR is still relevant to keep open, since the same fix is already included in the Pinot library upgrade PR?

imjalpreet · 2025-09-11T13:33:24Z

@ShahimSharafudeen, we can close this in favour of #25785

prestodb-ci added the from:IBM PR from IBM label Aug 18, 2025

ShahimSharafudeen marked this pull request as ready for review August 19, 2025 10:11

ShahimSharafudeen requested a review from a team as a code owner August 19, 2025 10:11

prestodb-ci requested review from a team, jkhaliqi and libianoss and removed request for a team August 19, 2025 10:11

ShahimSharafudeen requested a review from imjalpreet August 19, 2025 10:11

imjalpreet reviewed Aug 19, 2025

View reviewed changes

ShahimSharafudeen force-pushed the cve-fix-helix-core branch from 4efb71c to 91e2e21 Compare August 21, 2025 07:03

imjalpreet reviewed Aug 21, 2025

View reviewed changes

ShahimSharafudeen force-pushed the cve-fix-helix-core branch from 91e2e21 to c5b0647 Compare August 21, 2025 11:31

imjalpreet reviewed Aug 25, 2025

View reviewed changes

Upgrade helix-core to 1.4.3 to address CVE-2023-38647

9b586ff

ShahimSharafudeen force-pushed the cve-fix-helix-core branch from c5b0647 to 9b586ff Compare August 25, 2025 14:39

imjalpreet approved these changes Aug 28, 2025

View reviewed changes

tdcmeehan reviewed Aug 29, 2025

View reviewed changes

imjalpreet closed this Sep 11, 2025

	<dependency>
	<groupId>org.apache.pinot</groupId>
	<artifactId>presto-pinot-driver</artifactId>
	<version>${dep.pinot.version}</version>
	</dependency>

	<!-- Add the helix-core dependency to the root pom.xml to upgrade the transitive helix-core version used by the Presto Pinot driver-->
	<!-- Upgrades the transitive helix-core version used by the Presto Pinot driver to address CVE-2023-38647 -->

Conversation

ShahimSharafudeen commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Aug 19, 2025

Uh oh!

ShahimSharafudeen commented Aug 19, 2025

Uh oh!

ShahimSharafudeen commented Aug 19, 2025

Uh oh!

imjalpreet commented Aug 20, 2025

Uh oh!

ShahimSharafudeen commented Aug 21, 2025

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet commented Aug 29, 2025

Uh oh!

tdcmeehan commented Aug 29, 2025

Uh oh!

imjalpreet commented Aug 31, 2025

Uh oh!

ShahimSharafudeen commented Sep 9, 2025

Uh oh!

imjalpreet commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ShahimSharafudeen commented Aug 18, 2025 •

edited

Loading